Add --pppd-call option. #270

gbon121 · 2018-03-30T15:23:23Z

On systems where pppd supports the "call" option --- eg. Debian derived
distros --- privileged options to pppd can be moved to a config file owned
by root, and any unprivileged user in group "dip" can invoke openfortivpn.

Static routes and DNS settings are managed by /etc/ppp/ip-up.local and
/etc/ppp/ip-down.local scripts, provided the following lines are added
to /etc/openfortivpn/config:

set-routes = 0
set-dns = 0
pppd-ipparam = openfortivpn
pppd-call = openfortivpn

On systems where pppd supports the "call" option --- eg. Debian derived distros --- privileged options to pppd can be moved to a config file owned by root, and any unprivileged user in group "dip" can invoke openfortivpn. Static routes and DNS settings are managed by /etc/ppp/ip-up.local and /etc/ppp/ip-down.local scripts, provided the following lines are added to /etc/openfortivpn/config: set-routes = 0 set-dns = 0 pppd-ipparam = openfortivpn pppd-call = openfortivpn

DimitriPapadopoulos

What happens to this piece of code when openfortivpn is called by an unprivileged user?
https://github.com/adrienverge/openfortivpn/blob/25b2585/src/tunnel.c#L126-L146

DimitriPapadopoulos · 2018-03-30T20:09:58Z

src/ipv4.c

+		snprintf(*target + l0, l1, fmt, dest, mask, gw);
+	} else {
+		int eno = errno;
+		log_error("Could not reallocate array: %s\n", strerror(eno));


Not that the message is any better this way, just for mere consistency with the rest of the code:

log_error("realloc: %s\n", strerror(eno));

DimitriPapadopoulos · 2018-03-30T20:11:28Z

src/ipv4.c

+		*target = ptr;
+		snprintf(*target + l0, l1, fmt, dest, mask, gw);
+	} else {
+		int eno = errno;


While log_error() may theoretically change the value of errno, for mere consistency with the rest of the code I suggest getting rid of eno and using errno directly.

Actually log_error() cannot change the value of errno before it is used as the strerror(errno) argument is evaluated before the function is called.

DimitriPapadopoulos · 2018-03-30T20:22:14Z

The man page should eventually be updated too: doc/openfortivpn.1.in

gbon121 · 2018-03-31T07:02:01Z

On Fri, Mar 30, 2018 at 08:17:27PM +0000, Dimitri Papadopoulos Orfanos wrote: DimitriPapadopoulos commented on this pull request. What happens to this piece of code when _openfortivpn_ is called by an unprivileged user? https://github.com/adrienverge/openfortivpn/blob/25b2585/src/tunnel.c#L126-L146 > + char **target = &tunnel->config->pppd_ipparam; + char *ptr; + + if (*target == NULL || strncmp(*target, trigger, strlen(trigger))) + return; + if (!dest || !mask || !gw) + return; + log_info("Registering route %s/%s via %s\n", dest, mask, gw); + l0 = strlen(*target); + l1 = strlen(fmt) + strlen(dest) + strlen(mask) + strlen(gw) + 1; + if ((ptr = realloc(*target, l0 + l1))) { + *target = ptr; + snprintf(*target + l0, l1, fmt, dest, mask, gw); + } else { + int eno = errno; + log_error("Could not reallocate array: %s\n", strerror(eno));

Basically, (unprivileged) openfortivpn builds a list of routes to add after the tunnel is up, and passes the list to (privileged) pppd via the ipparam option, which is usually employed to pass auxiliary data available at runtime.

DimitriPapadopoulos · 2018-04-03T09:05:28Z

I must be missing something, but aren't all these options passed to pppd when they should be read from /etc/ppp/peers/openfortivpn instead?

		static const char *args[] = {
			pppd_path,
			"38400", // speed
			":1.1.1.1", // <local_IP_address>:<remote_IP_address>
			"noipdefault",
			"noaccomp",
			"noauth",
			"default-asyncmap",
			"nopcomp",
			"receive-all",
			"nodefaultroute",
			"nodetach",
			"lcp-max-configure", "40",
			"mru", "1354",
			NULL, // "usepeerdns"
			NULL, NULL, NULL, // "debug", "logfile", pppd_log
			NULL, NULL, // "plugin", pppd_plugin
			NULL, NULL, // "ipparam", pppd_ipparam
			NULL, NULL, // "ifname", pppd_ifname
			NULL // terminal null pointer required by execvp()
		};
		[...]
		execv(args[0], (char *const *)args);

DimitriPapadopoulos · 2018-04-03T09:23:41Z

Yes, I was definitely missing something! Perhaps some comments should be added to explain that --pppd-call overwrites the args array except for the pppd path.

DimitriPapadopoulos · 2018-04-03T09:29:07Z

I was thinking an if might be cleaner, something like this:

		if (tunnel->config->pppd_call) {
			...
		} else {
			...
		}

However I don't see how to initialize args and iin a more elegant way.

DimitriPapadopoulos · 2018-04-03T09:38:02Z

src/tunnel.c

@@ -145,6 +145,14 @@ static int pppd_run(struct tunnel *tunnel)
 			NULL // terminal null pointer required by execvp()
 		};

+		if (tunnel->config->pppd_call != NULL) {


Just write:

if (tunnel->config->pppd_call) {

Again I do not care myself but but the checkpatch.pl script of the Linux kernel emits a warning and we try to follow the Linux kernel coding style.

DimitriPapadopoulos · 2018-04-03T09:48:12Z

Looks good to me. Could you perhaps squash the commits so that I can merge your pull request?

DimitriPapadopoulos · 2018-04-03T09:58:44Z

Does this mean openfortivpn will not need to be run by root anymore on Debian-based systems? If so this looks like a solution for issue #54. We should probably disable --pppd-plugin and --pppd-log when pppd is not run by root or at least emit a warning when these options are used. Does this make sense?

No need to address the above issue in this pull request, we shall handle it in a different pull request.

On systems where pppd supports the "call" option --- eg. Debian derived distros --- privileged options to pppd can be moved to a config file owned by root, and any unprivileged user in group "dip" can invoke openfortivpn. Static routes and DNS settings are managed by /etc/ppp/ip-up.local and /etc/ppp/ip-down.local scripts, provided the following lines are added to /etc/openfortivpn/config: set-routes = 0 set-dns = 0 pppd-ipparam = openfortivpn pppd-call = openfortivpn

gbon121 · 2018-04-03T12:54:47Z

@DimitriPapadopoulos I'm not sure I squashed my commits properly. Should the PR result more difficult to merge than before, please let me know.

DimitriPapadopoulos · 2018-04-03T13:00:03Z

Actually you've added on more commit instead of replacing all existing commits by a single one, but don't worry: I can squash these commits myself not that I have the definitive comment (the one in the last commit).

gbon121 · 2018-04-11T16:26:33Z

@DimitriPapadopoulos I'm not sure the following code is more elegant of the current static array, but it might silence some warnings from Coverity:

#include <stdio.h>
#include <stdlib.h>
#include <assert.h>

struct v_arr {
        /* current capacity */
        unsigned cap;
        /* next slot to write to, always < max(cap - 1, 1) */
        unsigned off;
        /* NULL terminated */
        void **data;
};

void append_v_arr(struct v_arr *p, void *x) {
        if (p->off + 1 >= p->cap) {
                void **ndata;
                unsigned ncap = (p->off + 1) * 2;
                assert(p->off + 1 < ncap);
                ndata = realloc(p->data, ncap * sizeof(void *));
                if (ndata) {
                        p->data = ndata;
                        p->cap = ncap;
                } else {
                        abort();
                }
        }
        assert(p->off + 1 < p->cap);
        p->data[p->off] = x;
        p->data[++p->off] = NULL;
}       

void clear_v_arr(struct v_arr *p) {
        free(p->data);
}
        
void use_array_of_char(const char **arr) { 
        const char **p;
        printf("result:");
        for (p = arr; *p; ++p) 
                printf(" %s", *p);
        printf("\n"); 
}               
        
int main() {
        const char *frag[] = { "pppd", "option1", "option2", "option3" };
        struct v_arr v = { 0, 0, 0 };
        unsigned i; 
        for (i = 0; i < sizeof frag / sizeof *frag; i++)
                append_v_arr(&v, (void *)frag[i]);
        use_array_of_char((const char **)v.data);
        clear_v_arr(&v);
        return 0;
}

DimitriPapadopoulos reviewed Mar 30, 2018

View reviewed changes

gbon121 added 2 commits March 31, 2018 08:43

simplified use of log_error() and errno

764d7df

Updated manpage to mention the new option

4072b3e

DimitriPapadopoulos reviewed Apr 3, 2018

View reviewed changes

gbon121 added 2 commits April 3, 2018 11:43

Explain changes to pppd args[] when --pppd-call is specified.

025dea7

Adapt to the Linux kernel style.

58ffeeb

gbon121 added 2 commits April 3, 2018 14:27

DimitriPapadopoulos merged commit a2dfcb6 into adrienverge:master Apr 3, 2018

DimitriPapadopoulos mentioned this pull request Apr 4, 2018

Add script option #153

Closed

mrbaseman mentioned this pull request Apr 12, 2018

Command-line switches should have priority over config file #258

Closed

mrbaseman mentioned this pull request Apr 25, 2018

Reworked array of pppd args #295

Merged

gbon121 mentioned this pull request Apr 28, 2018

FreeBSD support #300

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --pppd-call option. #270

Add --pppd-call option. #270

gbon121 commented Mar 30, 2018

DimitriPapadopoulos left a comment

DimitriPapadopoulos Mar 30, 2018 •

edited

Loading

DimitriPapadopoulos Mar 30, 2018 •

edited

Loading

DimitriPapadopoulos Mar 30, 2018

DimitriPapadopoulos commented Mar 30, 2018

gbon121 commented Mar 31, 2018 via email

DimitriPapadopoulos commented Apr 3, 2018

DimitriPapadopoulos commented Apr 3, 2018

DimitriPapadopoulos commented Apr 3, 2018

DimitriPapadopoulos Apr 3, 2018

DimitriPapadopoulos commented Apr 3, 2018

DimitriPapadopoulos commented Apr 3, 2018 •

edited

Loading

gbon121 commented Apr 3, 2018

DimitriPapadopoulos commented Apr 3, 2018

gbon121 commented Apr 11, 2018

Add --pppd-call option. #270

Add --pppd-call option. #270

Conversation

gbon121 commented Mar 30, 2018

DimitriPapadopoulos left a comment

Choose a reason for hiding this comment

DimitriPapadopoulos Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

DimitriPapadopoulos Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

DimitriPapadopoulos Mar 30, 2018

Choose a reason for hiding this comment

DimitriPapadopoulos commented Mar 30, 2018

gbon121 commented Mar 31, 2018 via email

DimitriPapadopoulos commented Apr 3, 2018

DimitriPapadopoulos commented Apr 3, 2018

DimitriPapadopoulos commented Apr 3, 2018

DimitriPapadopoulos Apr 3, 2018

Choose a reason for hiding this comment

DimitriPapadopoulos commented Apr 3, 2018

DimitriPapadopoulos commented Apr 3, 2018 • edited Loading

gbon121 commented Apr 3, 2018

DimitriPapadopoulos commented Apr 3, 2018

gbon121 commented Apr 11, 2018

DimitriPapadopoulos Mar 30, 2018 •

edited

Loading

DimitriPapadopoulos Mar 30, 2018 •

edited

Loading

DimitriPapadopoulos commented Apr 3, 2018 •

edited

Loading